-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make initial incremental/watch builds as fast normal builds #42960
Conversation
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise. |
Update:
|
8225bf2
to
b926fcf
Compare
@sheetalkamat @weswigham I guess that's pretty difficult to review, since there are many changed files. If it helps we can hop on a call to walk through together. Or if there is anything else I can help with, just let me know. |
We will be discussing this in design meeting on whether this should be default or opt in behavior. After that i will review the changes. But looking at the change, I feel like we don't need to populate signature at all just mark in state if all source files are changed files (because there was no oldState and some other conditions that mark all files as need emit or invalidate semantic cache etc) then we shouldn't compute the signature otherwise compute it. That seems like simpler and correct implementation. Again I haven't reviewed it at all, which I will do after the design meeting so that I have all inputs to review it. |
https://github.com/microsoft/TypeScript/compare/lazySignatureCompute .. i just created commit with what meant when i said we can simplify this, obviously I haven't spent time looking at the test failures or change in deep to see if its correct and satisfies 0what you propose, Note that as part of design meeting #43069 we discussed that we will make this default without any option to disable this behavior. Add flag later if people ask for it. |
I see what you mean. That's probably a better way to solve that, compared to my added
That's great. My PR currently adds a |
Please do
Please remove the option and fix test cases .. you may want to add more scenarios if they were testing initial local change, then add another local change as next step to ensure that still works correctly from then on Also can you please pull out deduplication part in separate PR and add test that shows it for easier review and maintenance. Thank you for great idea and work |
Should I also move fix CommonJs modules no longer affecting the global scope |
That would be great. Thanks. |
b926fcf
to
c261f19
Compare
There is one case not fully covered by the benchmark. In our project, we can easily repro the slow initial pass of watch/incremental. We also have another case that is very slow, may not be as slow as the initial pass, but slower than those benchmark test cases (those that take 8-9 seconds with or without the fix). The case is: |
change tscWatch incremental test to test for lazy shape computation
9fc3260
to
c1d5384
Compare
c1d5384
to
c4fb4f3
Compare
@sheetalkamat rebased |
@sokra thanks. Will be reviewing this today sometime later after I manage DT queue and some other PR reviews. |
Awesome. Just tell me if there is anything I can help you with. |
@sokra please dont bother updating the PR with conflicts, i am looking into simplifying this change and will update you once i have some results. Meanwhile i am using this branch for the tests you have already taken efforts to update. |
ok. It's a little bit hard to look at these conflicts and don't wanting to fix them... Maybe you could look at the baselines of |
The major change of this PR has been merged as #43314 |
Is it released? |
The latest beta version includes the fix |
I vote for having this feature optional. I am encountering some errors after these release. I have a very complex typing system and I don't think it would be possible to reproduce the error in a simpler way. It would be nice to have a flag also in order to test if this is the reason. |
@ndr47 Can you provide more details? What functionality would you like to be optional? This PR has evolved quite a bit since it was created, but I believe the intention was to make some watch mode functionality lazier, which I wouldn't to expect non-watch builds at all (or affect the errors produced by watch builds - just the speed at which they are produced). I believe the most common reason for seeing different errors in different build modes is that compilation depends on input file ordering (e.g. because the same type is provided by two different files, possible versions of the same library). |
Another part merged here: #44090 |
Coming back to this after a long time -- are there any parts left over that haven't been merged as other PRs? |
@sandersn I wonder: is your question answerable at this point? |
I had forgotten about this PR. Regardless of whether its features have been included piecemeal over the past couple of years, it's so old that it would need to be restarted anyway. I'm going to close it. |
Current just running
tsc
is much faster compared totsc --watch
ortsc --incremental
. There are already multiple issues describing that problem (I didn't verify all of them if it's really the same problem):tsc --watch
extremely slow (regulartsc
compiles fine). #34916fork-ts-checker-webpack-plugin
with faster alternative vercel/next.js#13529After digging in the source code for a while I think I found the cause of that:
For incremental or watch builds typescript need to compute 2 additional things for each module:
the "shape" and the "referenced modules".
These things are needed to calculate which modules need to be invalidated when a file has changed.
Where a file has changed typescript calculates a new "shape" and if the new shape differs from the old one, it will follow the graph of "referenced modules" upwards and invalidates these modules too (recursively).
Currently typescript uses "emit as declaration file" to calculate the "shape" and "referenced modules" and this is basically as expensive as doing a full "emit". Most time of the initial build is spend with that.
So the initial incremental/watch build is as slow as running tsc with emit.
But tsc --noEmit is very fast compared to that.
Refactoring idea
I think we actually don't really need to compute the "shape" on initial build. The "shape" is only an optimization so that non-shape-affecting changes to the file don't invalidate importing modules.
I would propose to not compute the "shape" on initial build. When a change to the file happens use the module content (
version
) instead to check if we need to invalidate the parents. This will cause unnecessary invalidation if only internals has change, but that would be an acceptable trade-off (Keep in mind that typechecking is much faster than computing the shape).In addition to that compute the "shape" when a module was invalidated in cause of a file change or an "shape"-change of an referenced module (only real "shape" changes, don't do that when we haven't computed the old shape. This avoids computing too many shapes during a watch rebuild)
Note: Due to not computing the shape we also don't have access to
exportedModulesFromDeclarationEmit
and have to use all references of the module instead. This will cause the module to be invalidated more often until the invalidation is triggered by a real shape change, which will cause it to compute its own shape andexportedModulesFromDeclarationEmit
.Summary: Lazy compute "shapes" and "exported modules" on first invalidation. Without old shape and exported modules: Invalidate referencing modules on file change instead of shape change. Invalidate module if any referenced modules changes instead of only exported ones.
So that's what I did.
Benchmark
Running different test cases with a project with about 3000 files. Total time as reported by tsc. I did not do any averaging as the results are pretty clear:
with isolatedModules:
without isolatedModules
Raw data
Summary:⚠️ ), first time changing a file in watch/incremental mode takes a small hit (see ❗).
tsc --incremental
andtsc --watch
is now as fast a puretsc
(seeTest suite
All tests are passing. I updated a lot baselines as signatures are now missing from tsbuildinfo (they are
0
as marked for lazy computed), but there is no functional change.I needed to change some tests that verify that clean build and incremental build result in the same build info, which is no longer true when signatures are lazily computed.
I disabled lazy shape computation for
unittests:: tsserver:: compileOnSave
,unittests:: tsc-watch:: emit file --incremental
for somecompileOnSave
tests and for 8 tests usingassumeChangesOnlyAffectDirectDependencies
as the tests expect certain behavior that lazy shape computation would change. Note that the behavior is not wrong, but it doesn't fit to the test cases.Edge cases
There are a few edge cases one might run into:
A
Do a non shape affecting change to file that affects the global scope (and is not a declaration file).
Since we don't know it's non shape affecting on this first change this will need to typecheck all files.
The second change will no longer have this behavior since shape is then computed.
Note that all CommonJS files are currently considered as "affecting global scope", so this might be a problem for commonjs projects. I guess this is a bug and CommonJS modules should probably not flagged in this way.Note: I fixed that.B
Do a shape affecting change to a file that is referenced by many other modules.
On second change this will trigger a shape computation on all referencing modules, which might cause a extra delay (similar to the initial shape computation before this PR).
We could a limit in how many shapes should be computed at maximum during a single build to avoid this. But in worst case this would make it have the performance like the current initial builds have.
🔍 Search Terms
slow, incremental, watch
✅ Viability Checklist
My suggestion meets these guidelines:
Please verify that:
Backlog
milestone (required)master
branchgulp runtests
locally📃 Motivating Example
Incremental build are unattractive compared to full builds when using typescript for typechecking with
noEmit: true
.💻 Use Cases
next.js
Incremental builds are too slow. So you have to choose between:
--incremental
)--incremental
)Not using
--incremental
at allPS: tsbuildinfo reference list optimization
As a little extra I changed to serialization of tsbuildinfo a little bit so that duplicate lists of references are deduplicated (this is the first commit). This isn't strictly necessary, but in an intermediate version of this refactoring I just used all modules as fallback references and this resulted in an huge slowdown due to writing tsbuildinfo, so I optimized it a bit. I left it here, because it will decrease the tsbuildinfo size, which is good when it has to be transferred e. g. between CI builds.